Skip to content

Conversation

@asl
Copy link
Contributor

@asl asl commented May 18, 2025

No description provided.

@asl asl requested a review from fruffy May 18, 2025 22:12
@asl asl added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label May 18, 2025
@asl asl added the run-validation Use this tag to trigger a Validation CI run. label May 19, 2025
@asl
Copy link
Contributor Author

asl commented May 19, 2025

For now only core.p4 were marked as @corelib. I would probably suggest that backend-specific headers would use something like @corelib("foo") annotation so one could possibly distinguish the "source".

@vlstill
Copy link
Member

vlstill commented May 20, 2025

For now only core.p4 were marked as @corelib. I would probably suggest that backend-specific headers would use something like @corelib("foo") annotation so one could possibly distinguish the "source".

If there is an intention to use it in backend-specific libs, I would suggest using something like @lib or @p4lib instead of @corelib which seems to imply core.p4 to me. It might be also worth adding a mention to p4-spec if there is this intention.

Also, unless there is something preventing an end-user/programmer from adding this annotation (which would be tricky), there is no way to really rely on these (which is why various backends might use various hacks to detect their own externs :-/). But I think this is still probably a good thing to have.

@asl
Copy link
Contributor Author

asl commented May 20, 2025

Also, unless there is something preventing an end-user/programmer from adding this annotation (which would be tricky), there is no way to really rely on these (which is why various backends might use various hacks to detect their own externs :-/). But I think this is still probably a good thing to have.

Well, I would consider this similar to -isystem we're having in C/C++ world. Technically one could throw it onto arbitrary patch and e.g. suppress warnings from these "system" headers. I do not see large downsides from this, but would allow not to check include paths in the compiler to distinguish "built-in" externs from others.

@jafingerhut
Copy link
Contributor

Maybe I am missing some context here. Can you explain the proposed purpose and/or advantages of adding these annotations?

@asl
Copy link
Contributor Author

asl commented May 21, 2025

Maybe I am missing some context here. Can you explain the proposed purpose and/or advantages of adding these annotations?

The inability to distinguish system entities from user-defined ones is a long-standing problem in the compiler. Currently, we have the hacky solution where we check the include path, but that can be ambiguous based on user-configuration. Note that files like core.p4 are included via preprocessor, so there is no solid way from inside the compiler to distinguish, say, verify corelib extern from the user function of the same name (certainly one could try to check for argument types, parameter names, etc., but this is pretty verbose and specific).

@fruffy
Copy link
Collaborator

fruffy commented May 23, 2025

In terms of terminology, maybe system or builtin is a better term? I wonder whether this warrants a spec-change. We can merge this one as experimental feature.

Maybe I am missing some context here. Can you explain the proposed purpose and/or advantages of adding these annotations?

Basically what @asl said. A concrete use case are some passes where we remove declarations, eg.,

if (decl->getName().name == IR::ParserState::verify && getParent<IR::P4Program>())

We have hardcoded checks, which could be simplified if we have a policy not to remove system externs.

@ChrisDodd
Copy link
Contributor

ChrisDodd commented May 24, 2025

The inability to distinguish system entities from user-defined ones is a long-standing problem in the compiler. Currently, we have the hacky solution where we check the include path, but that can be ambiguous based on user-configuration.

IMO this is a feature, not a bug -- it should not matter where the code comes from, it should just matter what it says. The ONLY thing checking the source path (should be) done for is minimizing the debug P4 dumps when dumping IR as P4. In that case if a bunch of declarations come from the same source file, the can be replaced by a single #include.

Any other code that looks at the source file (other than for printing messages) is IMO wrong. The source file is only there for error messages and to aid debugging the compiler.

@fruffy
Copy link
Collaborator

fruffy commented May 24, 2025

The inability to distinguish system entities from user-defined ones is a long-standing problem in the compiler. Currently, we have the hacky solution where we check the include path, but that can be ambiguous based on user-configuration.

IMO this is a feature, not a bug -- it should not matter where the code comes from, it should just matter what it says. The ONLY thing checking the source path (should be) done for is minimizing the debug P4 dumps when dumping IR as P4. In that case if a bunch of declarations come from the same source file, the can be replaced by a single #include.

Any other code that looks at the source file (other than for printing messages) is IMO wrong. The source file is only there for error messages and to aid debugging the compiler.

Well, issues such as #3424 could be reverted again, which were necessary because distinguishing between persistent system methods and user-defined methods was error-prone.

@qobilidop
Copy link
Member

IMHO, there is a design flow with core.p4. The things defined there are not the usual "externs" I would assume in other programming languages. They are pretty much "built-ins", in the sense that their semantics are specified by the language spec (e.g. packet_in.extract), and the compiler needs to handle them in special ways.

That being said, converting core.p4 from externs to built-ins would be a significant breaking change for the P4 language, which might not be a practical solution any time soon.

The solution in this PR is much more practical.

Re @vlstill

If there is an intention to use it in backend-specific libs, I would suggest using something like @lib or @p4lib instead of @corelib which seems to imply core.p4 to me.

Agree with this.

@asl
Copy link
Contributor Author

asl commented May 25, 2025

So, for the name we're having:

  • @corelib
  • @lib
  • @p4lib
  • @system
  • @builtin

I do not have particular preference for the name. I'd like to have the door open for backend-specific externs with similar semantics (though it would be great to distinguish between core language externs and backend-provided, my thought was to have, say @corelib and @corelib(foo) with foo being a backend name). That said, I would probable lean to @lib or @builtin. Thoughts?

@qobilidop
Copy link
Member

My preference is to have 2 different annotations:

  1. Have one annotation reserved for core.p4 (and anything specified by the language spec). No particular preference among @corelib / @builtin / @system.
  2. Have a different annotation for backend-specific use cases. Maybe @lib(foo) / @p4lib(foo)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Topics concerning the core segments of the compiler (frontend, midend, parser) run-validation Use this tag to trigger a Validation CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants